Skip to content

Add OPcache restart hook - #15590

Merged
bwoebi merged 4 commits into
php:masterfrom
realFlowControl:florian/opcache-restart-hook
Sep 24, 2024
Merged

Add OPcache restart hook#15590
bwoebi merged 4 commits into
php:masterfrom
realFlowControl:florian/opcache-restart-hook

Conversation

@realFlowControl

@realFlowControl realFlowControl commented Aug 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hey folks,

adding to #15543 which allows access to OPcache shared globals, this hook will allow observing extensions to observe the actual OPcache restart using this code snippet:

void restart_hook(zend_accel_restart_reason reason)
{
    printf("IT HAPPENED\n");
}

void call_during_minit() {
    // assume you have the `opcache_handle` available
    void (**opcache_restart_hook)(zend_accel_restart_reason reason);
    opcache_restart_hook = DL_FETCH_SYMBOL(opcache_handle, "zend_accel_schedule_restart_hook");
    // do proper error handling ;-)
    *opcache_restart_hook = restart_hook;
}

CC: @arnaud-lb

@realFlowControl
realFlowControl marked this pull request as ready for review August 26, 2024 16:15
@realFlowControl
realFlowControl force-pushed the florian/opcache-restart-hook branch from 72527d9 to aa02ce7 Compare August 26, 2024 16:16
dstogov
dstogov requested changes Aug 27, 2024
View reviewed changes

@dstogov dstogov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this callback is really necessary:

  1. you should select a good place for it. I think it shouldn't be done under the exclusive lock.
  2. instead of DL_FETCH_SYMBOL() magic, it's better to declare the callback in zend.h and zend.c.

Comment thread ext/opcache/ZendAccelerator.c Resolved
Comment thread ext/opcache/ZendAccelerator.c Resolved
@realFlowControl

realFlowControl commented Sep 10, 2024

Copy link
Copy Markdown
Contributor Author
  1. you should select a good place for it. I think it shouldn't be done under the exclusive lock.

I'd need to try my prototype usage of that hook on x86 (so far only tested on aarch64), but the pointer that I can dlsym() from zend_smm_shared_globals is readable and that's all I need. I would also argue that it is a good thing, that the memory is write protected during the hook and hooked functions can not alter anything.

  1. instead of DL_FETCH_SYMBOL() magic, it's better to declare the callback in zend.h and zend.c.

It would remove the dlsym() call needed for someone using the hook, but it would also leak OPcache things into Zend, where it should not be IMHO. So far, there are no source code references from /Zend to /ext/opcache and I think this is a good thing. I would argue to leave the hook where it is, just to not leak OPcache things into other code where it should not be.

@realFlowControl

realFlowControl commented Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

Hey there @dstogov can you have a look again? Would be great having this in 8.4

@bwoebi
bwoebi merged commit 3293faf into php:master Sep 24, 2024
@realFlowControl
realFlowControl deleted the florian/opcache-restart-hook branch September 24, 2024 14:25
@nicolas-grekas nicolas-grekas mentioned this pull request Sep 21, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmb69 cmb69 cmb69 left review comments
@dstogov dstogov dstogov approved these changes
@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

No one assigned

Labels

ABI break Category: Engine Extension: opcache

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@realFlowControl @cmb69 @dstogov @bwoebi